docs(run-once): fix the examples that violate the key rule - #149
Merged
Conversation
… silently
runOnce keys its flag file by the string alone, in a directory shared by every
project in the run:
const flagDir = path.join(os.tmpdir(), `playwright-once-${process.ppid}`);
const flagFile = path.join(flagDir, `${key}.done`);
Nothing in the key comes from the project. So when one spec is matched by more
than one project -- which is what adding an `-app-next` lane does -- the first
project's setup satisfies the second, and the second skips configure() and
deploy() entirely. It then fails much later on a missing element, with nothing
pointing at the cause. That happened on rhdh-plugin-export-overlays#3318 and
was caught in review; four workspaces still pass literal keys today.
Automatic per-project scoping would be wrong, because both intents are real:
installing an operator into a fixed namespace that every project then uses
genuinely wants once per run, while anything touching a project's own namespace
wants once per project. The API cannot guess, so this makes the caller say:
await test.runOnce("my-setup", fn, { scope: "project" });
Default behaviour is unchanged.
For the keys that stay run-scoped, the dangerous case is no longer silent. The
flag file now records which project satisfied it, and a skip on behalf of a
*different* project logs a warning naming both and pointing at the option --
because that is nearly always the mistake rather than the intent.
Tests are new; there were none for this helper. Each was checked by mutation:
collapsing the scoped key back to `key` fails the per-project test, dropping the
warning guard fails the warning test, and warning unconditionally fails the
same-project test. Full suite 117/117, lint clean.
Part of RHIDP-16456.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Version Bump Check requires both for any change under src/, and it caught that I had pushed neither. Documents the new scope option and the silent skip it fixes. Part of RHIDP-16456. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI runs `yarn check`, which is typecheck + lint:check + prettier:check. I had run lint and the tests separately and missed the formatting gate, so the new test file failed it. `yarn check` is the command to run locally. Only the test file changed: prettier:check excludes docs/, so the changelog needed no reformatting, and running --write over it had touched two unrelated entries. Part of RHIDP-16456. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e applied
The guides still said a runOnce key must be unique across "all spec files
and projects", and their two flagship examples wrapped configure() and
deploy() with no scope — which is exactly the shape that leaves a second
project with no deployment. Someone reading the guide rather than the
changelog would have written the bug.
Both examples now pass { scope: "project" }, the key section explains what
scope decides instead of asserting the old rule, and a new Scope section
gives the run-vs-project table plus the reason deploy() needs no scope of
its own: its internal key already carries the namespace.
Also close the one silent path the option still had. With scope "project"
and no resolvable project the key fell back to the bare form and behaved as
run-scoped again, without a word. It now warns, and the unit test for it
fails when the warning is removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…project scope cannot be honoured
Review of the first two commits found the warning firing on the one case the
docs call correct, and the scope option still able to degrade quietly.
The warning fired on every cross-project skip of a run-scoped key, including
"install an operator into a fixed namespace every project then uses" — the
example the docs give for choosing "run". In an N-project run that printed N-1
warnings telling the author to switch to { scope: "project" }, which for that
setup would mean N operator installs, and there was no way to opt out because
the guard tested the resolved scope. It now tests `options.scope` as passed:
undefined means the author never thought about it, which is who the advice is
for; an explicit "run" is an answer and is left alone.
{ scope: "project" } with no resolvable project used to warn and then fall back
to the bare key — which is the shared-key bug it exists to prevent, reported by
one line easy to lose in CI output. It throws now. That only reaches callers
outside a Playwright context, since test.info() resolves inside beforeAll.
The flag file records which project satisfied a key, and the lock-free fast
path reads it from another process. writeFileSync truncates before it writes,
so a reader landing between the two steps saw an empty file and dropped the
warning — in the two-projects-one-spec case, the exact case it exists for.
Written through a temp file and renamed.
Docs: the earlier pass missed four files, including the whole overlay section,
which is the one rhdh-plugin-export-overlays follows and where the bug was
reported. Every documented runOnce block that wraps deploy() now passes the
scope — audited rather than spot-fixed — and the API reference documents the
third argument at all, which it did not. Also noted that nesting does not
rescue a missing scope: a run-scoped outer call skips before deploy() is
reached, so its internal protection never gets a say.
The changelog now leads with the fact that upgrading alone changes nothing:
the default is unchanged by design, so each affected call site has to opt in.
Both new behaviours were mutation-tested — reverting each turns exactly one
test red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Two review rounds landed on this branch since it opened. Summary for whoever picks it up. What changed in the behaviour
The warning no longer fires on a deliberate The flag file is written atomically. Its content is what names the project in the warning, and the lock-free fast path reads it from another process. DocumentationThe first pass missed four files, including the entire Also recorded that nesting does not rescue a missing scope — a run-scoped outer call skips before VerificationBoth new behaviours are mutation-tested — reverting each turns exactly one test red:
And end to end, one spec under two projects: Note the third column: an explicit One thing worth stating plainlyUpgrading to 2.1.10 does not fix an affected call site. The default is unchanged by design, so every existing Also: |
Member
|
Hey, I’m not sure we need this in the helper. In 2.1, the default one is the new front-end system, then why we are still running the old test cases in that way. If we have migrated successfully, then we can just remove the older one and use the new one, right? Overlay CI still runs both shells on the same spec (plugin and plugin-app-next), which is the collision we already hit (rhdh-plugin-export-overlays#3318). If we drop the legacy project once app-next is the default, that skip goes away. Until then two projects in one Playwright run still share one runOnce flag dir. Could we just put namespace in the key like deploy() already does (deploy-${namespace})? Something like runOnce isn’t tied to Playwright today, which is why deploy() and the unit tests can use it. { scope: "project" } plus test.info() couples it to a test, and defaulting to "run" means existing overlay keys stay broken until every caller opts in (a warning doesn’t actually run setup). What if we keep runOnce generic, update the examples, and let call sites do what deploy() already does? its easy and no worries of regression. |
Review is right and this reverts the code change entirely.
The rule this guide already stated was correct — "the key must be globally
unique across all spec files and projects... use a prefix that includes the
workspace or project name". What was wrong is that every example underneath it
then used a literal key around configure() and deploy(), violating the rule the
paragraph had just given. Someone reading the examples rather than the sentence
wrote the bug; the earlier version of this branch responded by changing the
sentence, which was the wrong half to move.
So runOnce goes back to what it was. The { scope: "project" } option, the
import of @playwright/test into a module that had no test-runner dependency,
and the version bump are all gone. deploy() and the unit tests can keep using
runOnce outside a Playwright context, which was the point of it being generic.
What lands is the documentation:
- the key section now says why a project-shared key breaks, quoting the flag
directory that is keyed on the runner PID alone, and shows the fix as
`${key}-${rhdh.deploymentConfig.namespace}` — which is what deploy() has
always done internally, and why deploy() was never affected;
- both intents are spelled out side by side, because both are real: a namespaced
key for setup that belongs to one project, a literal one for an operator
installed into a fixed namespace every project then uses;
- every example that wraps deploy() carries the namespace — twelve of them,
across the guide, the API reference, the deployment guide and all three
overlay pages;
- nesting is noted as no rescue: a project-shared outer key skips before
deploy() is reached, so its internal protection never gets a say.
Not carried over: the warning when a shared key is skipped for another project.
It needed test.info(), which is the coupling being reverted. That gap is better
closed in rhdh-plugin-export-overlays, where a static check can find a literal
key in a workspace whose config declares two projects over one spec — before a
deployment rather than after one.
RHIDP-16456.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gustavolira
added a commit
that referenced
this pull request
Aug 24, 2026
#149 was reduced to a documentation change, so it no longer bumps the version and 2.1.10 is free. The note about the two PRs colliding goes with it — there is only one version-bumping PR open now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gustavolira
changed the title
gustavolira
added a commit
that referenced
this pull request
Aug 24, 2026
#151 merged today and claimed 2.1.10, so the renumber in the previous commit collided with main and the Version Bump Check would have refused it. Same number as before, different reason: this is no longer about #149, which now bumps nothing at all. Worth noting separately: main says 2.1.10 but the publish workflow for it failed, so npm latest is still 2.1.9. If that is not resolved before this merges, 2.1.11 publishes over a 2.1.10 that never existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
You're right — reverted the whole code change. I had it backwards. The rule in the guide was already correct; what was broken is that every example under it used a literal key around Small thing, not to reopen it: "existing keys stay broken until callers opt in" is equally true of adding What does get lost is the warning that found the bad call sites, since it needed One back at you: dropping the legacy lanes once app-next is default is RHIDP-16460, still New and unassigned. Until someone owns it the two-project window stays open. |
Member
|
/lgtm |
subhashkhileri
pushed a commit
that referenced
this pull request
Aug 26, 2026
…oting legacy (#152) * fix(nfs): make a lane that is not actually running NFS fail instead of pass Three defects found while triaging the overlay migration, all with the same shape: the harness cannot tell a working NFS lane from one that quietly fell back to the legacy shell. RHIDP-16457 — a workspace could disable the NFS it asked for. The NFS secret layer merges before the workspace's own tests/config/rhdh-secrets.yaml, so a workspace setting APP_CONFIG_app_packageName or ENABLE_STANDARD_MODULE_FEDERATION for its own reasons overrode it. Nothing then failed: the legacy suite re-ran and passed, and the only thing the lane existed to prove was never exercised. deploy() now checks the merged secret still carries both markers and throws naming the key, the value found, and the file responsible. The merge order is untouched — a workspace has to be able to override defaults, which is exactly why the outcome needs checking rather than assuming. Also from RHIDP-16457, and from RHIDP-16461: configure() now logs which of the three mechanisms decided the shell, because none of them is visible from a single file and the answer currently requires reading a project name, an environment variable and a configure() call together. A namespace ending in -app-next that was explicitly configured with useNewFrontendSystem: false warns, since it deploys legacy under a name that reads as NFS everywhere. Only that direction is checked: enabling NFS without the suffix is legitimate and is what github and homepage do. RHIDP-16459 — the GitHub session file was a bare relative authState_<user>.json, resolved against process.cwd(), which the worker fixture sets to the same workspace directory for every project. A workspace's lanes shared one file per user, unlocked: one lane could inject another's storage state, and a reader landing mid-write died on truncated JSON as a flake that looked nothing like the plugin under test. Every lane added is another writer. The path is now absolute and keyed by project as well as user, the write goes through a temp file and a rename, and an unusable session falls through to a full login rather than throwing. RHIDP-16458 — SidebarTabs hardcoded "Self-service", which is the legacy shell's scaffolder title; under app-next the page is "Create", so a spec running there could not type-check without a cast. Both are members now. Deliberately only that: which label a lane should use is RHIDP-16462, three workarounds are in flight for it, and picking one here would be a fourth. All four new behaviours are mutation-tested — reverting each turns the matching tests red and nothing else. 136 tests pass; typecheck, eslint and prettier are clean; the docs build validates its own links. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(nfs): the session fix would have caused the failure it prevents Review found two of the three changes wrong in ways the tests did not reach. Keying the GitHub session file per project removed cross-lane sharing, which is what the ticket asked for — and would have made every lane of a workspace log in to the same GitHub account concurrently. logintoGithub derives its 2FA code from one shared TOTP secret, so lanes starting inside the same 30-second window submit the identical code and GitHub rejects the second. This file already carries retry handling for exactly that error, which is how it is known to happen; bulk-import would have gone from one login to three simultaneous ones. So the file is one per user again, and what was actually missing is added instead: a lock spanning read-or-login-and-write, not just the write, because two lanes that both decide there is no session go on to log in anyway. RHDH cookies from another lane were never the hazard — each lane's RHDH lives on its own namespace hostname. The [nfs] line was logged from configure(), but the worker fixture calls configure() with no arguments for every project before any spec runs. For github and homepage — the two lanes the change was written for, which opt in through configure({ useNewFrontendSystem: true }) — the first line said "off, from nothing", and the first line is the one a reader greps. Reported from deploy() now, once, after configuration is final. Two smaller ones. The guard accepted an unquoted YAML `true` by coercing in the comparison only, so it green-lit a payload the API server rejects with "cannot unmarshal bool into Go struct field ... of type string" — an opaque k8s failure in place of the guard's own message. It now rewrites the value as well. And the temp file survived a failed storageState write, accumulating untracked authState*.tmp files in a directory nothing gitignores them from. New tests for the lock, both write outcomes, and the boolean rewrite; the serialisation test waits for the first holder rather than racing two callers from the same tick, which was flaky. Mutation-tested: removing the lock, the temp-file cleanup, or the in-place coercion each turns exactly one test red. 141 pass, five consecutive runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: take 2.1.10, now that #149 claims no version #149 was reduced to a documentation change, so it no longer bumps the version and 2.1.10 is free. The note about the two PRs colliding goes with it — there is only one version-bumping PR open now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: back to 2.1.11 — main took 2.1.10 #151 merged today and claimed 2.1.10, so the renumber in the previous commit collided with main and the Version Bump Check would have refused it. Same number as before, different reason: this is no longer about #149, which now bumps nothing at all. Worth noting separately: main says 2.1.10 but the publish workflow for it failed, so npm latest is still 2.1.9. If that is not resolved before this merges, 2.1.11 publishes over a 2.1.10 that never existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(nfs): the deployment guide said the workspace secret always wins It does not any more, and the guide said it in as many words: "your rhdh-secrets.yaml still wins on conflicts". True for every key except the two NFS markers, where overriding now throws instead of quietly deploying the legacy shell — so the sentence this PR contradicts is corrected in the same change rather than left to be discovered from a stack trace. Also documented, because a new hard failure with no docs costs someone an hour: the `[nfs]` line and what to grep for when a lane behaves like the wrong shell, why it comes from deploy() rather than configure(), the exact throw and the fact that it is a post-condition on the caller's own config, and the -app-next namespace warning together with why the reverse is not checked. The dynamic-plugins layer is named as the unguarded one. Disabling app-auth or app-integrations from a workspace's own dynamic-plugins.yaml degrades the lane the same silent way, and nothing catches it. No workspace does that today — 14 ship their own dynamic-plugins.yaml and none names either package — so a second guard would be insurance against something nobody does, which is the argument that just closed rhdh-plugin-export-overlays#3375. Documented instead, and it is the first place to look when an NFS lane behaves like the legacy shell. deploy()'s step list gains both the report and the throw. 141 tests pass, yarn check clean, docs build clean. * fix(nfs): fail the lane on an intent conflict instead of warning about it The guard found the conflict and then let the run continue. A lane named <ws>-app-next configured with useNewFrontendSystem: false deployed the legacy shell, re-ran the legacy suite, passed, and left one console.warn behind on a run that exited 0 — which is indistinguishable from a run that had nothing to say. That is the exact failure this module exists to stop, and the PR's own title claims it does. assertNfsMarkersSurvived already throws for the other path, where a workspace's own rhdh-secrets.yaml overwrites the markers. The two paths now behave the same. describeNfsIntentConflict stays a pure describer with its existing tests; assertNfsIntentMatches is a thin wrapper over it so the throwing behaviour is directly testable rather than reachable only through a private method on RHDHDeployment. The [nfs] line is logged before the check so the resolved state is on record even when the next line ends the run. Mutation-verified: turning the throw back into a return fails "fails the deploy on the conflict, rather than only naming it" and nothing else. Closes the library half of RHIDP-16457 — bulk-import's local expect(rhdh.deploymentConfig.useNewFrontendSystem).toBe(true) was stronger than the library guard until now, which is the inverse of what that ticket asks for. * fix(github-session): lock only session creation, and scope this PR to that Two changes from review. Lock scope. The lock spanned read-or-login-and-write, so the reuse path — addCookies, goto, Sign In, wait for nav or popup — ran inside it too. Reuse needs no exclusivity: the lanes are different namespace hosts and only creation submits a TOTP. Every loginAsGithubUser therefore queued behind a sign-in it did not need, and a waiter could exhaust Playwright's default test timeout before proper-lockfile's own retries ran out, because test.setTimeout(260_000) is raised inside logintoGithub — precisely the path a waiter is not on. ensureGithubSession now checks for a session outside the lock and takes it only to create one, re-reading inside so a caller that queued behind the lane that created it reuses that session rather than logging in again with the same TOTP code. loginAsGithubUser splits into _reuseGithubSession and _createGithubSession; creation already leaves the page signed in, so the reuse path is not replayed after it. Mutation-verified, one test each: dropping the outer check fails "reuses an existing session without taking the lock"; dropping the inner re-read fails "creates once when two callers find no session at the same time". The first version of that first test was wrong and passed under the mutation — it started ensureGithubSession in the same tick as the lock holder, so it raced the scheduler rather than the lock and the result depended on how loaded the run was. It now waits until the lock is demonstrably held, and is bounded by a timer so reintroducing the bug fails instead of deadlocking CI. Scope. The NFS work is removed — nfs-guard, the deploy-time report and marker check, the SidebarTabs label, and the docs for them. With the old frontend coming out there is no legacy shell to silently fall back into, so guarding against it earns little and would be deleted in the cleanup anyway. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Documentation only after review — the API change is reverted.
git diff origin/main -- src/ package.jsonis empty.The guide already had the right rule:
Every example under it then used a literal key around
configure()+deploy(), breaking the rule the paragraph had just given. That is what was worth fixing.What changed
`${key}-${rhdh.deploymentConfig.namespace}`, which is whatdeploy()does internally and whydeploy()was never affected.deploy()is reached.Not here: the warning that found affected call sites needed
test.info(), which is the coupling being reverted. That moved to redhat-developer/rhdh-plugin-export-overlays#3375 as a static check.yarn checkclean, 111 tests, docs build clean.